Conversation
There was a problem hiding this comment.
Pull request overview
Updates lib/wsen-pads/README.md to bring WSEN-PADS documentation in line with the repo’s more complete driver READMEs by adding an API reference and documenting temperature calibration support.
Changes:
- Expanded the README with a structured API Reference covering initialization, measurements, raw reads, one-shot usage, continuous configuration, status, identification, power/reset, and calibration.
- Added a Calibration section documenting
set_temp_offset()andcalibrate_temperature(). - Reorganized/expanded introductory content (features list, I²C address section, notes, examples table).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nedseb
left a comment
There was a problem hiding this comment.
Salut Charly,
C'est un bon travail de fond — la documentation de l'API est beaucoup plus complète qu'avant, la structure est claire, et les sections Calibration et Power Management étaient effectivement manquantes. Voici les points à corriger avant de merger.
🔴 Problèmes à corriger
1. Ligne orpheline avant la section Examples
Il y a une ligne `sensor = WSEN_PADS(i2c)` en dehors de tout bloc de code, juste avant la section "# Examples" (visible dans le diff vers la ligne 447). C'est du code qui traîne — à supprimer.
2. Le tableau des exemples référence un fichier inexistant
| pressure.py | Basic pressure and temperature read |Le fichier `pressure.py` n'existe pas dans `lib/wsen-pads/examples/`. Les vrais fichiers sont :
- `basic_reader.py`
- `continuous_reader.py`
- `one_shot_reader.py`
- `altitude.py`
- `test.py`
Mets à jour le tableau avec les vrais noms. Un simple `ls lib/wsen-pads/examples/` t'aurait permis de le voir.
3. `exceptions.py` dans la structure du driver
└── exceptions.py
Cette fois c'est correct — le wsen-pads a bien un `exceptions.py` (contrairement au CONTRIBUTING.md de la PR #204 où c'était inventé). Vérifie toujours en faisant un `ls` du dossier avant de documenter l'arborescence.
4. Pas de newline finale
Le fichier se termine sans `\n`. Ajoute une ligne vide à la fin.
5. Import `Pin` inutilisé dans l'exemple Basic Usage
from machine import I2C, Pin`Pin` n'est pas utilisé dans l'exemple. Retire-le :
from machine import I2C🟡 Problèmes de précision
6. `read_one_shot(low_noise=True)` — documentation trompeuse
Le README documente :
sensor.read_one_shot(low_noise=True)Mais en regardant le code, `read_one_shot()` ignore complètement le paramètre `low_noise` — il appelle simplement `self.read()`. C'est en fait un bug du driver, pas de ta doc. Mais tu ne dois pas documenter un comportement qui ne fonctionne pas. Deux options :
- Documenter `read_one_shot()` sans le paramètre `low_noise`
- Ou noter que `low_noise` n'est supporté que via `trigger_one_shot()`
7. Section Hardware Connection supprimée
L'ancien README avait un tableau des pins (VDD, VDD_IO, GND, SDA, SCL, CS, SAO). Tu l'as supprimé. La section I²C Address conserve bien les adresses SAO LOW/HIGH, mais le tableau des pins physiques était utile pour quelqu'un qui câble le capteur. À reconsidérer — peut-être le garder dans une section réduite.
8. `read()` side effect non documenté
Le README présente `read()` comme une simple lecture. Mais `read()` appelle `_ensure_data()` qui appelle `trigger_one_shot()`, ce qui met le capteur en power-down puis déclenche une conversion. En mode continu, ça interrompt le mode continu. Ce comportement mérite une note, par exemple :
Note: `read()` triggers a one-shot conversion internally. In continuous mode, prefer `pressure_hpa()` and `temperature()`.
💡 Suggestions d'amélioration
9. Section Notes — bonne initiative
La section "Notes" à la fin est utile et contient des infos pratiques. Bien joué. La note sur le low-noise non disponible à 100/200 Hz est un bon détail.
10. Les séparateurs `---` entre chaque méthode
Comme dans la PR #204, les `---` entre chaque sous-section alourdissent la lecture. C'est un choix stylistique mais ça fait beaucoup de traits horizontaux. Les `###` suffisent à structurer. À toi de voir — ce n'est pas bloquant.
Résumé
| Problème | Sévérité |
|---|---|
| Ligne orpheline `sensor = WSEN_PADS(i2c)` | 🔴 À corriger |
| Exemple `pressure.py` inexistant | 🔴 À corriger |
| Pas de newline finale | 🔴 À corriger |
| Import `Pin` inutilisé | 🟡 À corriger |
| `read_one_shot(low_noise=...)` ne fonctionne pas | 🟡 Doc trompeuse |
| Section pins supprimée | 🟡 À reconsidérer |
| `read()` side effect non documenté | 🟡 Manque info |
| Séparateurs `---` excessifs | 💡 Style |
Globalement c'est un bon travail. La couverture API est complète, la calibration est bien documentée, et les exemples de code sont corrects. Corrige les 🔴, traite les 🟡, et ça sera prêt à merger.
|
Done : Ligne orpheline Should be ready to merge |
|
Tous les points de la revue ont été traités sauf la newline finale — corrigé en c366a1a. Note : cette branche contient aussi l'ancien README.md (avant la restructuration de la PR #204). Il y aura un conflit de merge à résoudre. Merci de rebaser sur main après le merge de #204, ou de résoudre le conflit manuellement. La PR est prête à merger (modulo le conflit README). |
c366a1a to
97da5d0
Compare
|
🎉 This PR is included in version 0.0.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #198
Parent issue #194
Add API Reference and calibration documentation to WSEN-PADS README
This PR completes the
lib/wsen-pads/README.mdby adding missing documentation sections:The README now aligns with the structure and completeness of other drivers such as
ism330dlandwsen-hids.